Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct Babel dependency behavior #5046

Merged
merged 2 commits into from
Sep 20, 2018

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Sep 20, 2018

Babel was compiling async/generators incorrectly in dependency using just preset-env. We need to rewrite these to use the non-global regenerator and apply other tweaks from the application configuration.

On a side note, it took me forever to figure out there was some bad actor in Yarn Workspaces/Babel that required me to git clean -fdx && yarn install every time I changed dependencies.js. Troubleshooting this took ages because of that.

@Timer Timer added this to the 2.0.0 milestone Sep 20, 2018
@Timer Timer merged commit a1a08db into facebook:next Sep 20, 2018
@Timer Timer deleted the hotfix/correct-babel-dependency-behavior branch September 20, 2018 20:24
@lixiaoyan
Copy link
Contributor

lixiaoyan commented Sep 21, 2018

@babel/plugin-transform-runtime will try to insert import statement by default ("module" is the default value of the sourceType config).

Before Babel:

async function fn() {}
module.exports = fn;

After Babel:

import _regeneratorRuntime from "@babel/runtime/regenerator";
import _asyncToGenerator from "@babel/runtime/helpers/asyncToGenerator";
// ...
module.exports = fn;

Mixing commonjs modules and es modules will cause webpack to generate invalid code.

@Timer
Copy link
Contributor Author

Timer commented Sep 21, 2018

Hmm, thank you for the information @lixiaoyan.
Do you have proposed better way of handling this?

@lixiaoyan
Copy link
Contributor

lixiaoyan commented Sep 21, 2018

@Timer Set sourceType to "unambiguous" for node_modules may resolve this issue but I haven't tried it yet.

@lixiaoyan
Copy link
Contributor

@Timer
Copy link
Contributor Author

Timer commented Sep 21, 2018

Awesome @lixiaoyan. Mind sending a PR changing this or do you not care for the git blame?

@Timer
Copy link
Contributor Author

Timer commented Sep 21, 2018

Even prepped the diff for you:

diff --git a/packages/react-scripts/config/webpack.config.dev.js b/packages/react-scripts/config/webpack.config.dev.js
index 12b8f8cf..0775b62a 100644
--- a/packages/react-scripts/config/webpack.config.dev.js
+++ b/packages/react-scripts/config/webpack.config.dev.js
@@ -277,6 +277,11 @@ module.exports = {
                   ],
                   cacheDirectory: true,
                   highlightCode: true,
+                  // Babel assumes ES Modules, which isn't safe until CommonJS
+                  // dies. This changes the behavior to assume CommonJS unless
+                  // an `import` or `export` is present in the file.
+                  // https://github.com/webpack/webpack/issues/4039#issuecomment-419284940
+                  sourceType: 'unambiguous',
                 },
               },
             ],
diff --git a/packages/react-scripts/config/webpack.config.prod.js b/packages/react-scripts/config/webpack.config.prod.js
index 1e0fadbf..6c44af5c 100644
--- a/packages/react-scripts/config/webpack.config.prod.js
+++ b/packages/react-scripts/config/webpack.config.prod.js
@@ -306,6 +306,11 @@ module.exports = {
                   ],
                   cacheDirectory: true,
                   highlightCode: true,
+                  // Babel assumes ES Modules, which isn't safe until CommonJS
+                  // dies. This changes the behavior to assume CommonJS unless
+                  // an `import` or `export` is present in the file.
+                  // https://github.com/webpack/webpack/issues/4039#issuecomment-419284940
+                  sourceType: 'unambiguous',
                 },
               },
             ],

@lixiaoyan
Copy link
Contributor

I think the best place to add this option is "babel-preset-react-app/dependencies.js" (it just works).
#5052

@Timer
Copy link
Contributor Author

Timer commented Sep 21, 2018

Makes sense. PR merged. 🎉 Thank you!

@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants